CCCT-2440 Connect App Launch For Remaining Pages And Back Navigation#3756
CCCT-2440 Connect App Launch For Remaining Pages And Back Navigation#3756conroy-ricketts wants to merge 17 commits into
Conversation
[AI] Routed the delivery and learning progress fragments through the silent launch path by extracting the shared progress-dialog and outcome-routing wiring into ConnectAppLaunchUiController and delegating all three Connect launch surfaces to it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
[AI] Addressed launch-controller review findings by injecting ConnectAppLauncher, extracting a pure LaunchProgressMapper with unit tests, self-dismissing the dialog via a view-lifecycle observer, and exposing named learn/delivery launch entrypoints. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
[AI] Fixed the silent launch path dropping the "View Job Status" redirect by launching Home for result and routing REDIRECT_TO_CONNECT_OPPORTUNITY_INFO to the seated app's job status page, mirroring DispatchActivity's legacy handling. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
[AI] Fixed Connect app back navigation so app Home backs out to the opportunities list (ending the app session) and "View Job Status" opens the job-status page on top of the live Home, preserving back-to-Home; reworked the prior for-result redirect handling accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
[AI] Addressed review feedback: renamed ConnectAppLaunchUiController to ConnectAppLaunchController, drove the launch dialog from the full LaunchDialogState, removed dead code (the unused launchHome and password-override helpers), removed the "silent" wording, and tightened comments. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Suggested Review Order
|
📝 WalkthroughWalkthroughThis PR refactors the Connect app launch flow by extracting repeated launch orchestration logic from multiple fragments into a shared Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/org/commcare/activities/HomeScreenBaseActivity.java`:
- Around line 589-596: In userPressedOpportunityStatus() in
HomeScreenBaseActivity, handle the null case from
ConnectJobHelper.INSTANCE.getJobForSeatedApp(this) by adding user feedback
and/or logging: if job is null, call Android Toast (or Snackbar) to inform the
user (e.g., "No active job found") and add a process or Android log entry (Log.w
or your logger) noting that getJobForSeatedApp returned null; retain the
existing call to ConnectNavHelper.INSTANCE.goToActiveInfoForJob(this, job, true)
when job is non-null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c93923c9-e88c-4f39-907f-e8e2e6d2f5c3
📒 Files selected for processing (10)
RELEASES.mdapp/src/org/commcare/activities/HomeScreenBaseActivity.javaapp/src/org/commcare/connect/ConnectAppLaunchController.ktapp/src/org/commcare/connect/ConnectAppUtils.ktapp/src/org/commcare/connect/ConnectNavHelper.ktapp/src/org/commcare/fragments/connect/ConnectDeliveryProgressFragment.javaapp/src/org/commcare/fragments/connect/ConnectJobsListsFragment.javaapp/src/org/commcare/fragments/connect/ConnectLearningProgressFragment.javaapp/unit-tests/src/org/commcare/connect/LaunchProgressMapperTest.ktdocs/commcare/login-engine.md
💤 Files with no reviewable changes (1)
- app/src/org/commcare/connect/ConnectAppUtils.kt
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3756 +/- ##
=========================================
Coverage 25.88% 25.88%
+ Complexity 4388 4384 -4
=========================================
Files 949 950 +1
Lines 57129 57163 +34
Branches 6804 6811 +7
=========================================
+ Hits 14786 14795 +9
- Misses 40517 40538 +21
- Partials 1826 1830 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
[AI] Logged a non-fatal workflow entry when "View Job Status" is pressed but no Connect job is found for the seated app, instead of silently doing nothing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…om:dimagi/commcare-android into CCCT-2440-login-silent-launch-path-for-remaining-connect-pages
…om:dimagi/commcare-android into CCCT-2440-login-silent-launch-path-for-remaining-connect-pages
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
I noticed a couple behavioral changes with the new approach that I think we'll need to address.
-
Back nav after launching app from learning/delivery progress pages
Old behavior: navigating back from App Home (after successful login) returned to opportunity list
New behavior: Back nav now returns to learning/delivery progress instead -
Logout button on App Home
Old behavior: Logs out all the way to the Login page
New behavior: Returns to previous page (opportunity list, learning/delivery progress, etc.)
…om:dimagi/commcare-android into CCCT-2440-login-silent-launch-path-for-remaining-connect-pages
Great catch! I didn't think to test that
I'm having trouble reproducing this point. Are you able to provide repro steps? |
|
No problem, these steps will show the different behavior:
Results:
|
|
Did a pull on the latest code and the back nav is working as expected now, so only the Logout button behavior still to address. |
[AI] Routed app-home logout to the login screen through a reused DispatchActivity (CLEAR_TOP | SINGLE_TOP) so it no longer drops back onto the Connect page or flashes a white screen. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…om:dimagi/commcare-android into CCCT-2440-login-silent-launch-path-for-remaining-connect-pages
…-2440-login-silent-launch-path-for-remaining-connect-pages
| // Launch the seated app's job status page on top of this (still-live) Home so the app | ||
| // session is preserved and backing out of the status page returns here. | ||
| ConnectJobRecord job = ConnectJobHelper.INSTANCE.getJobForSeatedApp(this); | ||
| if (job == null) { |
There was a problem hiding this comment.
Blocker: This seems crash-worthy to me, we should never show the Job Status button if not in a job context so the user clicking that button would indicate something is quite wrong.
There was a problem hiding this comment.
Great point
| private int initialTabPosition = 0; | ||
| private boolean isProgrammaticTabChange = false; | ||
| private ConnectDeliveryProgressViewModel viewModel; | ||
| private final ConnectAppLaunchController launchController = new ConnectAppLaunchController(this); |
There was a problem hiding this comment.
Seems like this could be a local variable in navigateToDeliverAppHome, or maybe use a static function instead?
There was a problem hiding this comment.
The reason for this not being a local variable or static is because ConnectAppLaunchController calls fragment.registerForActivityResult() when the fragment is initialized
So keeping it global here avoids an IllegalStateException crash (i.e. the fragment attempting to registerForActivityResult after being created)
| ); | ||
| launchDialog.addProgressBar(); | ||
| if (isLearning) { | ||
| launchController.launchLearningApp(appId); |
There was a problem hiding this comment.
Nit: Feels like we're splitting a boolean just to combine it back together, i.e. here we switch on isLearning to either call launchLearningApp or launchDeliveryApp, and those two functions just hard-code true and false for isLearning before calling launch(LaunchTarget(appId, isLearning)). A simple launchApp(appId, isLearning) seems like it would be more straight-forward.
There was a problem hiding this comment.
Yeah I was a little torn about this at first, and looking back at it again I agree your suggestion is better
| onHomeResult(result) | ||
| } | ||
|
|
||
| fun launchLearningApp(appId: String) = launch(LaunchTarget(appId, isLearning = true)) |
There was a problem hiding this comment.
Referenced in another comment but adding more detail here:
In all 3 usages of this class, we construct the class at fragment startup and then only use it in one place (technically two for he jobs list fragment but they're right next to each other and logically are like a single usage).
Is there a reason for the class to construct the launch controller before it's needed and hold a reference to it in the fragment? Otherwise I think a static launch method would be simpler and cleaner for callers.
[AI] Enforced a non-null job in userPressedOpportunityStatus via Objects.requireNonNull, since the Job Status button is only shown when a job exists for the seated app. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
[AI] Replaced launchLearningApp/launchDeliveryApp with a single launchApp(appId, isLearning) on ConnectAppLaunchController and removed the boolean-splitting helper in ConnectJobsListsFragment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
[AI] Documented the logout (RESULT_OK) outcome routing to the login screen via a reused DispatchActivity. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
CCCT-2440
Product Description
Extends the no-login-screen Connect app launch to the remaining priority entry points and makes back navigation between the app and Connect consistent:
Screen_Recording_20260609_113107_CommCare.Debug.mp4
Technical Summary
ConnectJobsListsFragmentinto a sharedConnectAppLaunchController, and routes all three Connect launch surfaces (jobs list, delivery progress, learning progress) through it.RESULT_CANCELED) ends the app session and returns to the opportunities list; a logout (RESULT_OK) routes to the login screen through a reusedDispatchActivity.userPressedOpportunityStatus("View Job Status") now launches the job-status page on top of the still-live home instead of finishing it, preserving back-to-home.LaunchProgressMapper→LaunchDialogState; dialog cleanup is handled by a view-lifecycle observer so callers don't manage it.HomeScreenBaseActivity.launchHomeandConnectAppUtils.getPasswordOverride/shouldOverridePassword).Safety Assurance
Safety story
What gives confidence:
LaunchProgressMapper,ConnectAppLauncher, andLaunchOutcomeRouterhave JVM unit-test coverage.Risks to review:
userPressedOpportunityStatusis shared with the legacyLoginActivitylaunch path; it now keeps home alive and launches the job page on top for that path too.ConnectAppUtils.getPasswordOverride/shouldOverridePasswordafter confirming no remaining source callers.Automated test coverage
LaunchProgressMapperTestcovers the per-phase dialog state mapping (seating / signing-in / syncing, percent handling, message override).ConnectAppLauncherTestandLaunchOutcomeRouterTestcover the launcher sequencing and outcome routing the controller delegates to.